Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache Structure Dictionaries #2922

Merged
merged 5 commits into from
Nov 7, 2019

Conversation

PaulTaykalo
Copy link
Collaborator

@PaulTaykalo PaulTaykalo commented Oct 26, 2019

What

  • Add wrapper over [String: SourceKitRepresentable] -> SourceKittenDictionary
  • Cache File.structure.dictionary

Why

  • This is one of the most often called and time-consuming function after the regex matching
    image
  • Speed up mostly
  • Swiftlint using readonly dictionaries from SourceKitten this allows to pre-cache all information used in Swiftlint
  • Swiftlint accesses substructure key too often, and since it is a computed property, too many objects will be created and removed upon accessing it
  • Having own type instead of extensions over Dictionary will also speed up indexer and compiler a bit

Note

This is more of a suggestion. I totally understand that this is a big change.
Some minor stylistic improvements can be done( like namings and moving extensions in and out)
i.e. this one probably should be moved to File.structurekinds
https://github.com/realm/SwiftLint/pull/2922/files#diff-74fa1a3ab98dac161c19949597728f07R9

This solution calculates all substructures of the original dictionary. Technically, this can lead to regression in time in some cases, if substructure is not used in rules.

This is also can lead to some code duplication, in places, where it is still easier to work with [Strings: SourekitRepresentable]


BTW in order to have backward compatible API, we should probably use duplicated methods which still works with [String: SourceKitRepresentable] and will wrap those under the hood.


Please see #2924 and #2929 for the more improvements, based on this solution

@PaulTaykalo PaulTaykalo added the discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. label Oct 26, 2019
init(_ value: [String: SourceKitRepresentable]) {
self.value = value

let substructure = value["key.substructure"] as? [SourceKitRepresentable] ?? []
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caching is mostly what this PR is about

@SwiftLintBot
Copy link

SwiftLintBot commented Oct 26, 2019

2 Warnings
⚠️ Big PR
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 2.08s vs 2.25s on master (7% faster)
📖 Linting Alamofire with this PR took 3.48s vs 3.95s on master (11% faster)
📖 Linting Firefox with this PR took 11.75s vs 12.76s on master (7% faster)
📖 Linting Kickstarter with this PR took 28.3s vs 29.56s on master (4% faster)
📖 Linting Moya with this PR took 2.32s vs 2.6s on master (10% faster)
📖 Linting Nimble with this PR took 2.6s vs 2.71s on master (4% faster)
📖 Linting Quick with this PR took 1.08s vs 1.1s on master (1% faster)
📖 Linting Realm with this PR took 3.55s vs 3.87s on master (8% faster)
📖 Linting SourceKitten with this PR took 1.69s vs 1.75s on master (3% faster)
📖 Linting Sourcery with this PR took 4.1s vs 4.65s on master (11% faster)
📖 Linting Swift with this PR took 18.98s vs 20.75s on master (8% faster)
📖 Linting WordPress with this PR took 30.95s vs 32.94s on master (6% faster)

Generated by 🚫 Danger

@PaulTaykalo PaulTaykalo force-pushed the speedup/cache-structure-dictionaries branch 2 times, most recently from f628909 to c687c22 Compare October 26, 2019 07:20
Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2019

Please add an entry in the "breaking changes" section of the changelog since this breaks backwards compatiblity with the API.

@PaulTaykalo PaulTaykalo force-pushed the speedup/cache-structure-dictionaries branch 2 times, most recently from 9aa0aea to 796cac6 Compare November 7, 2019 07:59
@PaulTaykalo PaulTaykalo force-pushed the speedup/cache-structure-dictionaries branch from 796cac6 to 4fff698 Compare November 7, 2019 08:28
@PaulTaykalo PaulTaykalo merged commit f845af6 into master Nov 7, 2019
@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2019

Thanks for the updates and merging! Can you delete branches after you merge PRs if you don't need the branch any more?

@PaulTaykalo PaulTaykalo deleted the speedup/cache-structure-dictionaries branch November 7, 2019 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants